Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make RecurrencePattern.Count, .Interval and WeekDay.Offset nullable to avoid using MinValue as magic number. #667

Merged
merged 5 commits into from
Dec 10, 2024

Conversation

minichma
Copy link
Collaborator

@minichma minichma commented Dec 9, 2024

Make the following properties nullable to avoid using MinValue as magic number:

  • RecurrencePattern.Count
  • RecurrencePattern.Interval
  • WeekDay.Offset

Closes #471

Copy link

codecov bot commented Dec 9, 2024

Codecov Report

Attention: Patch coverage is 70.37037% with 8 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...alization/DataTypes/RecurrencePatternSerializer.cs 50% 4 Missing and 2 partials ⚠️
Ical.Net/Evaluation/RecurrencePatternEvaluator.cs 75% 1 Missing and 1 partial ⚠️

Impacted file tree graph

@@         Coverage Diff         @@
##           main   #667   +/-   ##
===================================
- Coverage    63%    63%   -0%     
===================================
  Files        99     99           
  Lines      4592   4594    +2     
  Branches   1086   1085    -1     
===================================
+ Hits       2876   2877    +1     
+ Misses     1261   1258    -3     
- Partials    455    459    +4     
Files with missing lines Coverage Δ
Ical.Net/Constants.cs 44% <ø> (ø)
Ical.Net/DataTypes/RecurrencePattern.cs 74% <100%> (-1%) ⬇️
Ical.Net/DataTypes/WeekDay.cs 74% <100%> (+20%) ⬆️
Ical.Net/Evaluation/TodoEvaluator.cs 47% <100%> (ø)
...l.Net/Serialization/DataTypes/WeekDaySerializer.cs 78% <100%> (ø)
Ical.Net/Evaluation/RecurrencePatternEvaluator.cs 71% <75%> (-<1%) ⬇️
...alization/DataTypes/RecurrencePatternSerializer.cs 68% <50%> (-2%) ⬇️

@minichma
Copy link
Collaborator Author

minichma commented Dec 9, 2024

@axunonb I had this in the pipeline and as you just did the same for Until in #666 I thought I'd make a PR quickly, before we do the work twice. After both have been merged, it would be the time to remove the MinValue stuff from RecurrencePatternSerializer.CheckMutuallyExclusive (#470, #471) I assume.

@minichma minichma force-pushed the work/minichma/feature/magic_numbers branch from 13c9f2e to 6576096 Compare December 9, 2024 19:34
Copy link

sonarqubecloud bot commented Dec 9, 2024

@minichma minichma marked this pull request as ready for review December 10, 2024 11:32
@minichma minichma requested a review from axunonb December 10, 2024 11:32
Copy link
Collaborator

@axunonb axunonb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and sorry for interfering re UNTIL

@minichma minichma merged commit fb18177 into main Dec 10, 2024
6 of 8 checks passed
@minichma minichma deleted the work/minichma/feature/magic_numbers branch December 10, 2024 16:32
@minichma
Copy link
Collaborator Author

LGTM, and sorry for interfering re UNTIL

Oh, no worries, I didn't do anything with UNTIL, just wanted to avoid that you would implement the things from this PR, which would have been double work. Thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants